snapenv: Include snap application environment state - #16016
Conversation
759b4d3 to
9b0e8bf
Compare
|
Mon Jun 1 06:32:57 UTC 2026 Failures:Preparing:
Executing:
Restoring:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16016 +/- ##
=======================================
Coverage 78.98% 78.99%
=======================================
Files 1381 1377 -4
Lines 191967 191970 +3
Branches 2464 2464
=======================================
+ Hits 151630 151641 +11
- Misses 31184 31185 +1
+ Partials 9153 9144 -9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| // Set various SNAP_ environment variables as well as some non-SNAP variables, | ||
| // depending on snap confinement mode. Note that this does not include environment | ||
| // set by snap-exec. | ||
| for k, v := range snapEnv(info, component, opts) { | ||
| for k, v := range snapEnv(info, app, component, opts) { |
There was a problem hiding this comment.
does it make sense to have similar information for hooks?
type HookInfo struct {
Snap *Info
// Component will be nil if the hook is not a component hook.
Component *Component
Name string
Plugs map[string]*PlugInfo
Slots map[string]*SlotInfo
Environment strutil.OrderedMap
CommandChain []string
Explicit bool
}
looks like we could have SNAP_HOOK=<hook-name>
There was a problem hiding this comment.
Yeah, look like they could be useful too, although if an hook is running I assume it know that it knows what its own name?
There was a problem hiding this comment.
in theory the same applies to apps 😄
There was a problem hiding this comment.
Well... Not much, in the sense that while hooks are designed to be inside a snap, an application can be embedded into anyof snap
9b0e8bf to
c9bbf46
Compare
c9bbf46 to
fba145d
Compare
a33207e to
301f31b
Compare
|
@olivercalder if you can give a look it would be appreciated too :) |
olivercalder
left a comment
There was a problem hiding this comment.
LGTM, thanks! Just needs tests to be expanded a bit more, I think.
| MATCH "^SNAP_CONTEXT=$CTX" < snap-vars.txt | ||
| # parallel-installs: $SNAP_NAME is always _the_ snap name | ||
| MATCH '^SNAP_NAME=test-snapd-tools$' < snap-vars.txt | ||
| MATCH '^SNAP_APP_NAME=env$' < snap-vars.txt |
There was a problem hiding this comment.
Would be nice to test a snap which has all the new variables set.
There was a problem hiding this comment.
Eh... Could you hep with that? Not being able to run the spread tests myself makes it a bit annoying to do...
There was a problem hiding this comment.
You can run them with run-spread garden:... just install image-garden snap.
There was a problem hiding this comment.
test-snapd-desktop-file-ids should do the trick, but needs a rebase on master. I'll poke at this and see what I can do.
60791f2 to
22eb166
Compare
zyga
left a comment
There was a problem hiding this comment.
LGTM
Please expand the integration test to show all variables at least once.
Please update the documentation, especially on what to do as an app developer to get the correct experience.
| "SNAP_APP_NAME": app.Name, | ||
| } | ||
|
|
||
| if app.CommonID != "" { |
There was a problem hiding this comment.
This is all very welcome. We must document it somewhere.
| MATCH "^SNAP_CONTEXT=$CTX" < snap-vars.txt | ||
| # parallel-installs: $SNAP_NAME is always _the_ snap name | ||
| MATCH '^SNAP_NAME=test-snapd-tools$' < snap-vars.txt | ||
| MATCH '^SNAP_APP_NAME=env$' < snap-vars.txt |
There was a problem hiding this comment.
You can run them with run-spread garden:... just install image-garden snap.
There was a problem hiding this comment.
Pull request overview
This PR extends the snap run-time environment to expose per-application identity/state (not just per-snap), so desktop environments and app integrations can reliably determine which snap app is running (per Launchpad #2125222).
Changes:
- Add app-scoped SNAP_* variables (starting with
SNAP_APP_NAME, plus optional common ID, bus name, and desktop file path) to the environment assembled forsnap run. - Thread the app context into
snapenv.ExtendEnvForRun(...)fromsnap runso the env can be populated for app runs while remaining nil-safe for hooks/components. - Update unit and spread tests to validate
SNAP_APP_NAMEand adjust expected variable counts.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/main/snap-env/task.yaml | Asserts SNAP_APP_NAME is present and updates expected SNAP_* variable counts. |
| snap/snapenv/snapenv.go | Adds app-level env population (SNAP_APP_*) and updates the run-env API to accept an app context. |
| snap/snapenv/snapenv_test.go | Adds unit tests covering app env vars, including desktop-file resolution scenarios. |
| cmd/snap/cmd_run.go | Passes the runnable app context into snapenv.ExtendEnvForRun when building the execution environment. |
When running an application of a snap is complicated to get some per-app state values such as its name, desktop file and common IDs, but these are actually important to make the application to be able to identify itself properly in a desktop environment. As per this, expose the application information as env variables. Related to: https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/2125222
olivercalder
left a comment
There was a problem hiding this comment.
I think we can properly test things now with the test-snapd-desktop-file-ids snap.
| MATCH "^SNAP_CONTEXT=$CTX" < snap-vars.txt | ||
| # parallel-installs: $SNAP_NAME is always _the_ snap name | ||
| MATCH '^SNAP_NAME=test-snapd-tools$' < snap-vars.txt | ||
| MATCH '^SNAP_APP_NAME=env$' < snap-vars.txt |
There was a problem hiding this comment.
test-snapd-desktop-file-ids should do the trick, but needs a rebase on master. I'll poke at this and see what I can do.
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
22eb166 to
b35217b
Compare
olivercalder
left a comment
There was a problem hiding this comment.
I rebased and extended the spread test slightly, otherwise looks good to me.
| MATCH '^SNAP_NAME=test-snapd-desktop-file-ids$' < snap-vars-desktop.txt | ||
| MATCH '^SNAP_APP_NAME=cmd$' < snap-vars-desktop.txt | ||
| MATCH '^SNAP_APP_COMMON_ID=org.example.Foo$' < snap-vars-desktop.txt | ||
| MATCH '^SNAP_APP_DESKTOP_FILE=/var/lib/snapd/desktop/applications/org.example.Foo.desktop$' < snap-vars-desktop.txt |
There was a problem hiding this comment.
AFAICT there are no existing spread tests which check the bus name, except a few use python scripts, which seems a stretch to use here. But we can at least check the other new desktop metadata.
even better, I've opened canonical/snap-docs#354 |
When running an application of a snap is complicated to get some per-app state values such as its name, desktop file and common IDs, but these are actually important to make the application to be able to identify itself properly in a desktop environment.
As per this, expose the application information as env variables.
Related to: https://bugs.launchpad.net/ubuntu/+source/snapd/+bug/2125222